Skip to content

test DoctrineOrmPropertyMetadataFactory#773

Merged
dunglas merged 1 commit into
api-platform:masterfrom
soyuka:test-doctrine-metadata
Sep 29, 2016
Merged

test DoctrineOrmPropertyMetadataFactory#773
dunglas merged 1 commit into
api-platform:masterfrom
soyuka:test-doctrine-metadata

Conversation

@soyuka

@soyuka soyuka commented Sep 28, 2016

Copy link
Copy Markdown
Member
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Made this because:

  1. there was none
  2. I needed to test something

@soyuka soyuka force-pushed the test-doctrine-metadata branch 2 times, most recently from e6e7e5e to ad81182 Compare September 28, 2016 14:03
foreach ($identifiers as $identifier) {
if ($identifier === $property) {
$propertyMetadata = $propertyMetadata->withIdentifier(true);
if ($doctrineClassMetadata instanceof ClassMetadataInfo) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change ? It looks wrong to me (isIdentifierNatural isn't part of the interface).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh gosh you're right, looked the wrong interface (Persistence vs ORM)

@soyuka soyuka force-pushed the test-doctrine-metadata branch 2 times, most recently from 6b2e55f to 2e1b562 Compare September 28, 2016 16:29

@Simperfit Simperfit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please rebase ?

@soyuka soyuka force-pushed the test-doctrine-metadata branch from 2e1b562 to 270bad3 Compare September 28, 2016 20:40
foreach ($identifiers as $identifier) {
if ($identifier === $property) {
$propertyMetadata = $propertyMetadata->withIdentifier(true);
$writable = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removing the else? It avoids an extra assignation.

@soyuka soyuka Sep 28, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$writable also gets assigned in the else. IMO it helps for readability.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but here it is assigned 2 times instead of 1 in most cases ($doctrineClassMetadata is almost always an instance of ClassMetadataInfo): first it is assigned to false, then it is instantly assigned to the value of $doctrineClassMetadata->isIdentifierNatural()).

@soyuka soyuka force-pushed the test-doctrine-metadata branch from 270bad3 to bb3af99 Compare September 29, 2016 08:18
@dunglas dunglas merged commit a958d71 into api-platform:master Sep 29, 2016
@dunglas

dunglas commented Sep 29, 2016

Copy link
Copy Markdown
Member

Thank you @soyuka

@soyuka soyuka deleted the test-doctrine-metadata branch November 3, 2016 13:44
magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants